Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some changes has been Done in LDA code #206

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Ritik-in-Tech
Copy link

References to other Issues or PRs or Relevant literature

This is the link of the code file https://colab.research.google.com/drive/1tCT28q7D3YlMnGmVLEiFWsAdT2hlW-ZY?usp=sharing

Brief description of what is fixed or changed

Other comments

@rohansingh9001
Copy link
Collaborator

@testgithubtiwari we cannot merge this PR in its current state. I would like to offer some suggestions to meet this repository's requirements.

I observe you have used sklearn, which in itself is a machine-learning library. Since we are aiming to implement algorithms from scratch, I advise you to avoid using them. Moreover functionality like test train split is very easy to implement in itself using numpy.

We also have our own copy of the iris dataset in the Examples/datasets directory. You can use that instead of sklearn's copy of iris.

In the latter half, you have also implemented the LDA algorithm from scratch. You only need to merge this code, not anything above that.

Moreover, you have also written some driver code. This should never be placed in the implementation modules as they will run when you import this module in some other python file.

Create a new file in the Examples folder and shift this driver code over there.

Lastly do not convert ipynb notebooks to code directly. Please use a proper text editor and use proper lifting (flake8 in this case) to check your code for lint mistakes. Follow the formatting scheme already present in the library, you can view other files for reference.

Lastly use pytest to check if all the tests are passing and addition of your code did not break anything. A good contribution also includes unit tests added for whatever functions and classes you added.

For the time being, you can look at the details of checks to see how you can correct the lint errors.

@Ritik-in-Tech
Copy link
Author

Thank's for your suggestion I try to meet these requirements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants